Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove mailbox from warp config #5312

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mshojaei-txfusion
Copy link
Collaborator

@mshojaei-txfusion mshojaei-txfusion commented Jan 28, 2025

Description

Refactors Warp Route configuration to make mailbox addresses optional during initial config creation. This change separates the mailbox configuration from the base token configuration, introducing new types WarpRouteDeployConfigWithoutMailbox and HypTokenRouterConfigWithoutMailbox. This improves the configuration workflow by allowing more flexible mailbox address handling.

Drive-by changes

  • Extracted common refinement and transformation logic into separate functions for better code organization
  • Improved type safety in configuration validation functions
  • Removed unused isAddress import

Related issues

#5237
#5258

Backward compatibility

Yes - This change maintains backward compatibility while providing more flexibility in configuration

Copy link

changeset-bot bot commented Jan 28, 2025

⚠️ No Changeset found

Latest commit: da36297

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mshojaei-txfusion mshojaei-txfusion changed the title Refactor/remove mailbox from warp config refactor: remove mailbox from warp config Jan 28, 2025
Copy link

codecov bot commented Jan 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.53%. Comparing base (f1f4ae4) to head (da36297).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5312   +/-   ##
=======================================
  Coverage   77.53%   77.53%           
=======================================
  Files         103      103           
  Lines        2110     2110           
  Branches      190      190           
=======================================
  Hits         1636     1636           
  Misses        453      453           
  Partials       21       21           
Components Coverage Δ
core 87.80% <ø> (ø)
hooks 79.39% <ø> (ø)
isms 83.68% <ø> (ø)
token 91.27% <ø> (ø)
middlewares 79.80% <ø> (ø)

Comment on lines +174 to +177
export const WarpRouteDeployConfigSchema = z
.record(HypTokenRouterConfigSchema)
.refine(refineTokens, WarpRouteDeployConfigSchemaErrors.NO_SYNTHETIC_ONLY)
.transform(transformRebaseConfig);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally prefer not to include drive-bys like this except because it makes what would otherwise be a very easy to review PR much harder to review
IIUC we have better test coverage on this now so relying on that exercising this

Copy link
Collaborator Author

@mshojaei-txfusion mshojaei-txfusion Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really sure what exactly you mean with drive-bys,
I refactored this section since both WarpRouteDeployConfigSchema and WarpRouteDeployConfigSchemaWithoutMailbox needed to share the same refine and transform functions.

Comment on lines +104 to +105
export const HypTokenRouterConfigWithoutMailboxSchema =
HypTokenConfigSchema.and(GasRouterConfigSchema.omit({ mailbox: true }));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think zod will not error on extraneous fields so wondering why not just remove the mailbox field from the existing schema?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understood correctly but it is not possible do remove mailbox from MailBoxClienSchema as it is required for other parts of code, also omit is not supported for HypTokenRouterConfigMailboxSchema

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

3 participants